Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

timers: fix timers with non-integer delay hanging. #8073

Closed
wants to merge 1 commit into from

Conversation

misterdjules
Copy link

When backporting f8193ab into v0.10, a regression was introduced. Timers
with non-integer timeout could trigger a infinite recursion with 100%
cpu usage. This commit backports 93b0624 which fixes the regression.

After backporting f8193ab, instead of using Date.now(), timers would use
Timer.now() to determine if they had expired. However, Timer.now() is
based on loop->time, which is not updated when a timer's remaining time
is > 0 and < 1. Timers would thus never timeout if their remaining time
was at some point > 0 and < 1.

With this commit, Timer.now() updates loop->time itself, and timers
always timeout eventually.

Fixes #8065 and #8068.

@chrisdickinson
Copy link

Looks good to me, would love to get @tjfontaine's +1. Great work!

When backporting f8193ab into v0.10, a regression was introduced. Timers
with non-integer timeout could trigger a infinite recursion with 100%
cpu usage. This commit backports 93b0624 which fixes the regression.

After backporting f8193ab, instead of using Date.now(), timers would use
Timer.now() to determine if they had expired. However, Timer.now() is
based on loop->time, which is not updated when a timer's remaining time
is > 0 and < 1. Timers would thus never timeout if their remaining time
was at some point > 0 and < 1.

With this commit, Timer.now() updates loop->time itself, and timers
always timeout eventually.

Fixes nodejs#8065 and nodejs#8068.
@misterdjules misterdjules changed the title timers: fix issue #8065. timers: fix timers with non-integer delay hanging. Aug 4, 2014
@tjfontaine
Copy link

Thanks landed in 6f04394

@florianreinhart
Copy link

I think I am running into this issue. Everything seems to work fine, but after a few days node just locks up with 100% CPU usage. In the debugger I can see that listOnTimeout() in timer.js is called repeatedly. In the while loop in this function diff is always smaller than msecs:

while (first = L.peek(list)) {
    var diff = now - first._monotonicStartTime;
    if (diff < msecs) {
      list.start(msecs - diff, 0);
      debug(msecs + ' list wait because diff is ' + diff);
      return;
    } else {
    // ...
}

Could you please confirm that I am running into the same issue discussed here? Or is this another problem?

@chrisdickinson
Copy link

@florianreinhart Yep, this is the same issue.

timoxley added a commit to timoxley/functional-javascript-workshop that referenced this pull request Aug 19, 2014
misterdjules pushed a commit to misterdjules/node that referenced this pull request Aug 26, 2014
PR nodejs#8034 came with a test to make sure that timers expiry is based on
monotonic time and not on wall-clock time. However, a bug in the
implementation broke timers with non-integer delays. A fix for this
issue was provided with PR nodejs#8073, but it didn't come with a test.

Because nodejs#8073 fixed a subtle issue that could reappear in the future,
and because the impact of such an issue would be significant, I suggest
adding this test.

The test would timeout after 1 minute if the issue was reproduced.
Otherwise it will run very quickly.
misterdjules pushed a commit to misterdjules/node that referenced this pull request Aug 27, 2014
PR nodejs#8034 came with a test to make sure that timers expiry is based on
monotonic time and not on wall-clock time. However, a bug in the
implementation broke timers with non-integer delays. A fix for this
issue was provided with PR nodejs#8073, but it didn't come with a test.

Because nodejs#8073 fixed a subtle issue that could reappear in the future,
and because the impact of such an issue would be significant, I suggest
adding this test.

The test would timeout after 1 minute if the issue was reproduced.
Otherwise it will run very quickly.
indutny pushed a commit that referenced this pull request Sep 2, 2014
PR #8034 came with a test to make sure that timers expiry is based on
monotonic time and not on wall-clock time. However, a bug in the
implementation broke timers with non-integer delays. A fix for this
issue was provided with PR #8073, but it didn't come with a test.

Because #8073 fixed a subtle issue that could reappear in the future,
and because the impact of such an issue would be significant, I suggest
adding this test.

The test would timeout after 1 minute if the issue was reproduced.
Otherwise it will run very quickly.

Reviewed-By: Fedor Indutny <[email protected]>
mscdex pushed a commit to mscdex/node that referenced this pull request Dec 25, 2014
PR nodejs#8034 came with a test to make sure that timers expiry is based on
monotonic time and not on wall-clock time. However, a bug in the
implementation broke timers with non-integer delays. A fix for this
issue was provided with PR nodejs#8073, but it didn't come with a test.

Because nodejs#8073 fixed a subtle issue that could reappear in the future,
and because the impact of such an issue would be significant, I suggest
adding this test.

The test would timeout after 1 minute if the issue was reproduced.
Otherwise it will run very quickly.

Reviewed-By: Fedor Indutny <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants